-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(sinks): log response body when HTTP response failed #24434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
d27d01f to
35f0bf7
Compare
thomasqueirozb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @keuin, thanks for your PR! While this functionality is good I fear it may also add CPU overhead to users which see repeated http failures and could become quite spammy. I think that this should be gated behind a configuration option and disabled by default.
We can then include the configuration option over at https://vector.dev/guides/developer/debugging (file website/content/en/guides/developer/debugging.md) so that users are able to easily turn this on/off to debug issues.
| "gzip" => Box::new(GzDecoder::new(body_bytes)), | ||
| "deflate" => Box::new(ZlibDecoder::new(body_bytes)), | ||
| "br" => Box::new(BrotliDecoder::new(body_bytes, BROTLI_INTERNAL_BUFFER_SIZE)), | ||
| "zstd" => Box::new( | ||
| ZstdDecoder::new(body_bytes).unwrap_or_else(|_| ZstdDecoder::new(body_bytes).unwrap()), | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure these would work given we're reading only partial data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this works because the truncation is applied on decompressed byte stream, not the original compressed data (body_bytes). The decompressors can always read as many bytes as they need.
I agree with you. This will indeed spam the log if there are repeated errors, because 1024 bytes is not short and will take multiple lines in screen. I will add a gate flag to disable this by default. |
35f0bf7 to
4b2e95f
Compare
4b2e95f to
1468193
Compare
|
I gated this feature with Disabled (default behavior), no new logs appeared: Enabled (when |
1468193 to
2a0dc89
Compare
Summary
This patch adds HTTP response body peek logic, reading for at most 1024 UTF-8 characters from HTTP response. It uses this utility function to log response from sink, when sink reports error. This helps user find out why the sink is not working more quickly.
Vector configuration
I use a local ClickHouse server as sink, standard input as source:
How did you test this PR?
I spin up the test sink ClickHouse, which talks to Vector via HTTP.
I created test table with:
I configured Vector to write to that table. Then I spin up Vector, wrote data in stdin:
{"value":"some_value"}Since the data cannot be converted to
Int64, ClickHouse reported an error. Without this patch, Vector won't tell you why. With this patch, Vector clearly showed what caused the error:Change Type
Is this a breaking change?
Does this PR include user facing changes?
no-changeloglabel to this PR.References
Notes
@vectordotdev/vectorto reach out to us regarding this PR.pre-pushhook, please see this template.make fmtmake check-clippy(if there are failures it's possible some of them can be fixed withmake clippy-fix)make testgit merge origin masterandgit push.Cargo.lock), pleaserun
make build-licensesto regenerate the license inventory and commit the changes (if any). More details here.